-
Notifications
You must be signed in to change notification settings - Fork 883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate deprecated wait.Poll function #3906
Conversation
Welcome @Affan-7! It looks like this is your first PR to karmada-io/karmada 🎉 |
0422064
to
0fecfba
Compare
/lgtm /assign @RainbowMango for workflow and approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
Thanks~
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3906 +/- ##
=======================================
Coverage 53.33% 53.33%
=======================================
Files 252 252
Lines 20482 20482
=======================================
Hits 10924 10924
Misses 8836 8836
Partials 722 722
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
747247b
to
0fecfba
Compare
The tests were failing, here are the logs: https://github.com/karmada-io/karmada/actions/runs/5783869909/job/15676511801#step:5:2635 Should we increase the default value of I can't run e2e tests locally, because they are too slow on my computer. |
It might be flak, let's give it another try. |
I'm not sure yet. let's give it another try and try to find the root cause. I'm curious that the test failed in Just echo the log here:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change all of the 🙂PollUntilContextTimeout
like I showed I belive tests would work.
Edit: PollWithContext
is deprecated too.
/remove-approve |
0fecfba
to
2954bcb
Compare
Here is what I did:Summary: Value of Longer explanation: I added Reference: https://github.com/Affan-7/karmada/actions/runs/5841035785/job/15840850551#step:5:2798 So I run the test again with the deprecated Reference: https://github.com/Affan-7/karmada/actions/runs/5841648595/job/15842010633#step:5:2999 The value of
Reference: https://pkg.go.dev/k8s.io/apimachinery/pkg/util/wait#Poll I was assuming that the default value of So I added the value of After specifying the value of |
Can you please merge this @RainbowMango 😊. Or is there something that should I improve? |
Thanks @Affan-7 for the excellent analysis, I think it is acceptable to set a timeout here. I wonder if this is affected by kubernetes/kubernetes#119533? I'm hesitant to do it before kubernetes/kubernetes#119533. |
Hi @Affan-7 We can get back on this now as we already updated the Kubernetes dependencies to v1.29+ which includes the fix we want. Please feel free to let me know if you can work on this. |
pkg/karmadactl/register/register.go
Outdated
@@ -509,7 +509,7 @@ func (o *CommandRegisterOption) constructKarmadaAgentConfig(bootstrapClient *kub | |||
} | |||
|
|||
klog.V(1).Infof("Waiting for the client certificate to be issued") | |||
err = wait.Poll(1*time.Second, o.Timeout, func() (done bool, err error) { | |||
err = wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, o.Timeout, false, func(ctx context.Context) (done bool, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why immediate
here is set to false
? condition
can be invoked immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is because this behavior is equipment with previous.
wait.Poll
always waits the interval before the run of condition
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get it, thanks~
In addition this, if try to get a issued certificate right after it is created, the likelihood of failure is higher, so choose to wait a interval before the run of the condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the idea not run the condition so rush.
pkg/karmadactl/unjoin/unjoin.go
Outdated
@@ -223,7 +223,7 @@ func (j *CommandUnjoinOption) deleteClusterObject(controlPlaneKarmadaClient *kar | |||
} | |||
|
|||
// make sure the given cluster object has been deleted | |||
err = wait.Poll(1*time.Second, j.Wait, func() (done bool, err error) { | |||
err = wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, j.Wait, false, func(ctx context.Context) (done bool, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto~
answered at the last comment.
Signed-off-by: Mohammed Affan <mohammed.affan.727@gmail.com>
2954bcb
to
6a07cca
Compare
Signed-off-by: RainbowMango <qdurenhongcai@gmail.com>
Just rebased this and fixed the golint issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks~
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR replaces the deprecated
wait.Poll()
function with thewait.PollUntilContextTimeout()
function.Which issue(s) this PR fixes:
Related to #3835
1 out of 4 tasks
Does this PR introduce a user-facing change?: